Skip to content

Conversation

@erikdw
Copy link
Contributor

@erikdw erikdw commented May 12, 2017

  1. Update from checkstyle-6.11.2 to checkstyle-7.7.
  2. Tweak google_checks.xml from checkstyle-7.7 to have the following changes that
    are more inline with the way the storm code is written:
    • 4 space indents instead of 2
    • line-length limit is 120 instead of 100
  3. Exclude the generated thrift code in storm-client from being checked.
  4. Update all maxAllowedViolations to be in-sync with the current number of
    violations, through use of the update-all-pom-violations.bash script that
    is attached to STORM-2510.

NOTE: this also fixes STORM-2507.

erikdw added 2 commits May 12, 2017 02:26
1. Update from checkstyle-6.11.2 to checkstyle-7.7.
2. Tweak google_checks.xml from checkstyle-7.7 to have the following changes that
are more inline with the way the storm code is written:
  * 4 space indents instead of 2
  * line-length limit is 120 instead of 100
3. Exclude the generated thrift code in storm-client from being checked.
4. Update all maxAllowedViolations to be in-sync with the current number of
violations, through use of the `update-all-pom-violations.bash` script that
is attached to STORM-2510.

NOTE: this also fixes STORM-2507.
@erikdw
Copy link
Contributor Author

erikdw commented May 12, 2017

@vinodkc & @revans2 & @hmcc & @srishtyagrawal : please 👀 when you have a chance. This is a follow-up to:

  1. An offline email thread about the changes in PR STORM-2495:Integrate checkstyle check during build #2093 causing STORM-2507, and
  2. also the discussions in PR [STORM-2507] Master branch build failure due to additional checkstyle violations in storm-webapp module #2105, and
  3. also the discussions in PR STORM-2421: support lists of childopts in DaemonConfig. #2083.

@revans2
Copy link
Contributor

revans2 commented May 12, 2017

Like I said before I am fine with whatever convention we have, so long as we have one.

+1

@revans2
Copy link
Contributor

revans2 commented May 12, 2017

Oops we need a license in storm-buildtools/storm_checkstyle.xml or we need to white list it for rat.

erikdw added 2 commits May 12, 2017 10:12
…), and add the Apache License boilerplate to the storm_checkstyle.xml file
@erikdw
Copy link
Contributor Author

erikdw commented May 12, 2017

@revans2 : ah, thanks for pointing out the need for license. I copied how google-cloud-intellij is doing it:

I also increased the line limit to 140 and adjusted the maxAllowedViolations accordingly.

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still +1

@harshach
Copy link
Contributor

+1

@asfgit asfgit merged commit b8d0402 into apache:master May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants